-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[TextField] Migrate FormHelperText to emotion #24661
Conversation
@material-ui/core: parsed: +0.17% , gzip: +0.19% |
I'm probably missing something obvious but I'm not sure these tests are failing. It seems like it's working when testing the component manually. |
@duganbrett when you say it's working manually, do you mean you try to do syle overrides and theme variants? |
I can ensure that the style overrides are working but the tests are failing. Here is a codesandbox - https://codesandbox.io/s/formpropstextfields-material-demo-forked-ke0sr?file=/demo.js Here are the failing tests - https://app.circleci.com/jobs/github/mui-org/material-ui/221043?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link I tried skipping the tests in JSDOM, and at least locally the |
test_browser was green on 935c276: https://app.circleci.com/pipelines/github/mui-org/material-ui/37358/workflows/1d716975-2d25-4189-ab36-89abcf9fb863/jobs/221058. What am I missing? Generally, rather push the failing build than a green build with skips. With a failing build you have the failure to start with. With a green build you don't know where to start without looking through all changes. |
@@ -0,0 +1,17 @@ | |||
export interface FormHelperTextClasses { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A question while I'm here: Do these need to be interfaces for module augmentation or would a Record<FormHelperTextClassKey, string>
be sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, I believe we can use the same. It will also ensure that the typings on the classses
are correct.__
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how we should best handle this. So far the components were re-introducing this new typing. In order not to block the migration, should we handle this change in the end for all components once they are migrated? I will leave a comment on the issue for this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we could try to migrate one component to this approach, and if it works well, migrate them all at once? I could do it this weekend if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me 👍 will merge this one then and we have this #24405 (comment) for reference of what needs to be done
…ett/material-ui into migrate-form-helper-text
@eps1lon I've linked the failing build - https://app.circleci.com/jobs/github/mui-org/material-ui/221043?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link thought it would be easier if I can link the failing and the commit that fixed it (skipped JSDOM). I've reverted the changes with 7b8124c the build should be identical with the one linked above |
But you've said test:karma was at leat passing locally so I suspected it's a problem with test_browser not test_unit. Failing build that's blocking is simpler. I just can inspect the last workflow instead of reading through comments. |
I suspect that it has to do with how styles cascade. That's probably not that clear from "since JSDOM does not implement the Cascade" in the error message. "Cascade" is jargon in CSS and describes how multiple style declarations for the same element interact with each other. Would love to link to https://developer.mozilla.org/en-US/docs/Web/CSS/Cascade in the error message. In a perfect world I would link the a more in-depth markdown text but I don't see how this results in more people trying to understand the problem. |
In will then prepare a PR where these tests (the conformance) will skip JSDOM. |
Yeah, I think running style related tests in JSDOM is futile. I don't think this will ever get better. But we might get a faster iteration speed for browser test to the point where it's faster to run test:karma locally. For now skipping JSDOM is fine. |
This PR migrates the FormHelperText component to the new emotion format as a part of #24405